feat(backup): add GKE backup configuration in the module #2270
Conversation
Ameausoone
force-pushed
the
feat/backup-plan
branch
from
0c8e37a to
98f63dc
Compare
Introduce new variables for GKE backup configuration, including `backup_cron_schedule`, `backup_rpo_target_in_minutes`, `backup_config`, and `backup_retain_days`. These new settings enable backup schedule definition, RPO configuration, volume data and secrets backup options, and backup retention period definition.
Ameausoone
force-pushed
the
feat/backup-plan
branch
from
98f63dc to
acb8692
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by adding GKE backup configuration to the module. The changes are consistently applied across various modules, which is great. However, I've found a critical issue in the Terraform configuration for the google_gke_backup_backup_plan resource. The use of two dynamic "backup_schedule" blocks is invalid and will cause Terraform to fail. I have provided a detailed comment with a suggested fix to address this. Additionally, I've included a minor suggestion to simplify a validation condition for one of the new variables.
| dynamic "backup_schedule" { | ||
| for_each = var.backup_cron_schedule != null ? [var.backup_cron_schedule] : [] | ||
| content { | ||
| cron_schedule = backup_schedule.value | ||
| } | ||
| } | ||
|
|
||
| dynamic "backup_schedule" { | ||
| # If both backup_schedule and rpo_config are specified, backup_schedule have the precedence | ||
| for_each = var.backup_rpo_target_in_minutes != null && var.backup_cron_schedule ==null ? [var.backup_rpo_target_in_minutes] : [] | ||
| content { | ||
| rpo_config { | ||
| target_rpo_minutes = backup_schedule.value | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The use of two dynamic "backup_schedule" blocks is invalid in Terraform and will cause a configuration parsing error, as a resource can only have one dynamic block of a given name. To correctly implement the mutual exclusivity and precedence for cron_schedule and rpo_config, you should use a single dynamic "backup_schedule" block with nested logic. I've also added a newline at the end of the file, which is a good practice.
dynamic "backup_schedule" {
for_each = var.backup_cron_schedule != null || var.backup_rpo_target_in_minutes != null ? [1] : []
content {
cron_schedule = var.backup_cron_schedule
dynamic "rpo_config" {
for_each = var.backup_cron_schedule == null && var.backup_rpo_target_in_minutes != null ? [1] : []
content {
target_rpo_minutes = var.backup_rpo_target_in_minutes
}
}
}
}
| type = number | ||
| default = null | ||
| validation { | ||
| condition = var.backup_rpo_target_in_minutes == null || try(var.backup_rpo_target_in_minutes >= 60 && var.backup_rpo_target_in_minutes <= 86400, false) |
There was a problem hiding this comment.
The try() function in this validation condition is unnecessary. Terraform's || operator uses short-circuit evaluation, so if var.backup_rpo_target_in_minutes is null, the expression on the right of the || will not be evaluated, preventing an error. You can simplify this condition.
condition = var.backup_rpo_target_in_minutes == null || (var.backup_rpo_target_in_minutes >= 60 && var.backup_rpo_target_in_minutes <= 86400)
apeabody
changed the title
|
(remove extra space from PR for commitlint) |
There was a problem hiding this comment.
Thanks for the contribution @Ameausoone!
Prior to review, can you please run make build and the other items listed in the Lint output, and add to this change. Thanks!
2 participants
Description
This pull request introduces new configuration variables for managing GKE backups:
backup_cron_schedule: Define backup scheduling as per a cron expression.backup_rpo_target_in_minutes: Configure the Recovery Point Objective (RPO).backup_config: Specify which volumes or secrets to back up.backup_retain_days: Set the retention period for backups.google_gke_backup_backup_planto enable backupCloses #2259
Checklist
No tests added, but I tested this PR manually, tests seems to not be designed to check other resources than the GKE cluster.
By the way
Integration tests are designed from the ground to be executed in a organisation (which is totally relevant), but it could be difficult (even for professional) to get project creation rights in an organisation. It would be handy to have at least a procedure to execute the tests, in a regular project.